containers: Make sure state can't go out of sync in the container class with ctx.container - #243

Model not found: cloudflare-ai-gateway/anthropic/claude-opus-4-7. Did you mean: anthropic/claude-opus-4.5, anthropic/claude-opus-4.6, anthropic/claude-opus-4.7?

github run

@gabivlj Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

Open in StackBlitz

npm i https://pkg.pr.new/@cloudflare/containers@243

commit: b4388c0

Model not found: cloudflare-ai-gateway/anthropic/claude-opus-4-7. Did you mean: anthropic/claude-opus-4.5, anthropic/claude-opus-4.6, anthropic/claude-opus-4.7?

github run

I'm Bonk, and I've done a quick review of your PR.

Guards getState()/syncPendingStoppedEvents()/alarm() against a concurrent in-flight start reporting a running container as stopped.

Findings (2 posted as inline suggestions):

1. getState() reads state before awaiting startInFlight (src/lib/container.ts:599) — the await can't influence the returned snapshot, so the stale-state case the PR targets still leaks through, plus it triggers a spurious setRunning() rewrite. Suggestion posted.
2. Unguarded await this.startInFlight in syncPendingStoppedEvents() (src/lib/container.ts:2126) — a rejected start now propagates into alarm() (skipping alarm cleanup and pending onStop), stop(), and startAndWaitForPorts(). Suggestion posted.

Not posted inline:

3. Re-entrancy deadlock via lifecycle hooks — doStartContainer's handleError calls await this.onError(err) (src/lib/container.ts:1830) while startInFlight is still pending. A subclass whose onError calls await this.getState() or await this.stop() now awaits the very promise it is running inside, hanging until the DO is evicted. This wasn't possible before the PR. Consider a re-entrancy flag set around the hook invocation (or around the whole start path) so the join is skipped when already inside it.
4. getState() latency (minor) — it can now block for the full start window (~8–20s) rather than returning immediately; worth calling out in the docstring since it's a public API previously treated as a cheap accessor.

The alarm early-return at src/lib/container.ts:2075 looks safe: alarm() already re-armed via setAlarm(Date.now()) at the top, so returning early leaves the loop alarm pending rather than dropping it. Changeset is present and patch is appropriate.

github run

I'm Bonk, and I've done a quick review of your PR.

Guards alarm() / syncPendingStoppedEvents() against an in-flight start, and makes getState() repair a stale stopped state when ctx.container.running is true.

Findings (2 posted as inline comments, 1 with a suggestion):

1. Blanket early-return in syncPendingStoppedEvents() can permanently drop a pending onStop (src/lib/container.ts:2114) — start() never drains pending stop events, so if an alarm fires while a start is in flight, the previous run's onStop (and its exit code) is skipped, and once the start succeeds no later alarm can detect it. Suggested narrowing the guard to the healthy/running branch only; the stopped_with_code branch is already race-safe via setStoppedIfUnchanged. Also makes stop() a silent no-op for stop events during a start.
2. getState() repair discards the recorded exitCode (src/lib/container.ts:610) — setRunning() overwrites stopped_with_code, downgrading a crash to onStop({ exitCode: 0 }). Also, getState() is now a public read API that writes to storage and resets lastChange; worth documenting.

Not posted inline (no reviewable line):

3. 8.8 MB compiled Go binary committed — examples/egress-interception/container_src/server is an ELF 64-bit executable with debug info. The example's Dockerfile already builds it from main.go, so it's unused and unrelated to this change; it will stay in git history permanently. Drop it and add a .gitignore entry.
4. examples/basic/pnpm-lock.yaml (+872) looks accidental — every example is npm-locked, and examples/basic already has a package-lock.json.
5. npx prettier --check src/tests/container.test.ts fails, which will break the format CI job.

Changeset is present and patch is appropriate. Unit tests pass (27/27).

github run

I'm Bonk, and I've done a quick review of your PR.

The force-push to cae4ffb cleaned up the stray Go binary and pnpm-lock.yaml (thanks), but src/lib/container.ts and src/tests/container.test.ts are byte-identical to ace96a1.

Findings (none posted inline — no reviewable line for #1, and #2/#3 are already published on this PR):

1. The changeset was dropped in the force-push. .changeset/keep-start-state-synchronized.md existed at ace96a1 but is gone at cae4ffb (.changeset/ now contains only README.md and config.json). This PR changes getState() — a src/index.ts-reachable public API — from a pure read into something that can write running to DO storage, so per AGENTS.md it needs a changeset. Looks accidental rather than intentional; re-add the file (patch bump is right).

2. Prior finding still open: the blanket startInFlight early-return in syncPendingStoppedEvents() (src/lib/container.ts:2114) can permanently drop a pending onStop and its exit code, and makes stop() a silent no-op for stop events during a start. Details in the existing inline comment on review 4995678929.

3. Prior finding still open: the getState() repair at src/lib/container.ts:610 calls setRunning() for stopped_with_code, discarding the recorded exitCode. Details in the same review.

Unit tests pass (27/27) on this commit.

github run

I'm Bonk, and I've done a quick review of your PR.

Guards alarm() / syncPendingStoppedEvents() against an in-flight start, and makes getState() repair a stale stopped state when ctx.container.running is true.

1 new finding posted inline (src/lib/container.ts:610-616), with a suggestion.

Status of previously published findings on this commit — both still apply unchanged, details in review 4995678929:

• src/lib/container.ts:2124 — blanket startInFlight early-return in syncPendingStoppedEvents().
• src/lib/container.ts:614 — setRunning() repair discarding the recorded exitCode, and getState() no longer being a pure read.

Resolved since the last review: the changeset is back (patch is right), the stray Go binary and pnpm-lock.yaml are gone, and the unguarded await this.startInFlight in syncPendingStoppedEvents() was replaced by an early return, so the rejected-start propagation into alarm() / stop() / startAndWaitForPorts() no longer applies.

Unit tests pass (27/27).

github run

I'm Bonk, and I've done a quick review of your PR.

Guards alarm() / syncPendingStoppedEvents() against an in-flight start, and makes getState() repair a stale stopped state when ctx.container.running is true.

No new findings on fefce9c. The only change since the last reviewed commit (4282c79) is a Prettier reflow of the getState should not wait for an in-flight start test signature in src/tests/container.test.ts; src/lib/container.ts is byte-identical. I did not submit a new review, since re-posting the same inline comments would just duplicate notifications.

Status of previously published findings — all three still apply unchanged:

1. Blanket startInFlight early-return in syncPendingStoppedEvents() (src/lib/container.ts:2118) — can permanently drop a pending onStop and its exit code, and makes stop() a silent no-op for stop events during a start. Details in review 4995678929. Worth noting the alarm() path makes this worse: if syncPendingStoppedEvents() skips the event and the start then fails before the this.startInFlight !== undefined check at line 2062, control falls through to deleteAlarm() — so the missed onStop loses its last chance to be re-detected.
2. getState() repair discards the recorded exitCode (src/lib/container.ts:614) — setRunning() overwrites stopped_with_code, and getState() is no longer a pure read. Same review.
3. getState() repair leaves the monitor detached (src/lib/container.ts:616) — a read can manufacture a synthesised onStop({ exitCode: 0 }) unpaired with any onStart. Details in review 4995840927.

Verification on this commit: npm run test:unit 27/27 pass, npm run typecheck clean, npm run lint clean, npx prettier --check 'src/**/*.ts' now passes. Changeset is present and patch is the right bump.

github run

I'm Bonk, and I've done a quick review of your PR.

Guards alarm() / syncPendingStoppedEvents() against an in-flight start, and makes getState() repair a stale stopped state when ctx.container.running is true.

1 new finding posted inline (src/lib/container.ts:615-616, with a suggestion): the new this.monitor ??= ...; this.setupMonitorCallbacks(); re-arm can register monitor callbacks during an in-flight start (the window between container.start() and await this.state.setRunning()). If the container then crashes on boot, the monitor's .finally clears this.monitor before handleError samples it, so handleError overwrites the recorded stopped_with_code with plain stopped, calls onError(undefined), and throws the literal undefined out of start(). Gating the re-arm on this.startInFlight === undefined keeps the exhaustion-path fix (that path always runs with startInFlight already cleared).

Status of previously published findings — both still apply unchanged on 7fead30:

• src/lib/container.ts:2120 — blanket startInFlight early-return in syncPendingStoppedEvents() can permanently drop a pending onStop and its exit code, and makes stop() a silent no-op for stop events during a start. Details in review 4995678929.
• src/lib/container.ts:614 — the setRunning() repair discards the recorded exitCode for stopped_with_code (undelivered onStop gets downgraded to exitCode: 0), and getState() is no longer a pure read. Same review. The new comment at 606-609 explains the intent but doesn't address the undelivered-onStop case.

Resolved since the last review: the detached-monitor gap from review 4995840927 is fixed by the re-arm (modulo the mid-start caveat above).

Verification on this commit: npm run test:unit 27/27 pass, npm run typecheck clean, npx prettier --check 'src/**/*.ts' clean. Changeset present, patch is the right bump.

github run

I'm Bonk, and I've done a quick review of your PR.

Guards alarm() / syncPendingStoppedEvents() against an in-flight start, and makes getState() repair a stale stopped state when ctx.container.running is true.

No new findings on b4388c0. The only diff since the last reviewed commit (7fead30) is the startInFlight === undefined gate around the monitor re-arm plus explanatory comments — that resolves the mid-start monitor-attachment finding from review 4995966.... I did not submit a new review, since the remaining findings are already published inline and re-posting would just duplicate notifications.

Status of previously published findings — both still apply unchanged:

1. Blanket startInFlight early-return in syncPendingStoppedEvents() (src/lib/container.ts:2129) — still the highest-severity item. start() never drains pending stop events, so an alarm firing during a start skips the previous run's onStop (and its real exit code); once the start succeeds, container.running is true and no later alarm can re-detect it. It also makes stop() (line 1022) a silent no-op for pending stop events while a start is in flight. Narrowing the guard to the healthy/running branch keeps the race fix — the stopped_with_code branch is already race-safe via setStoppedIfUnchanged. Suggestion in review 4995678929.
2. getState() repair discards the recorded exitCode (src/lib/container.ts:614-618) — the new comment documents the tradeoff for the "previous process, newer process already running" case, which is a defensible call. It doesn't cover the case the comment omits: a stopped_with_code whose onStop has not been delivered yet is downgraded to onStop({ exitCode: 0, reason: 'exit' }) by the next syncPendingStoppedEvents(). Restricting the repair to status === 'stopped', or draining the pending stop event before overwriting, avoids that. Also still worth a docstring note that getState() is no longer a pure read — it can write to DO storage and reset lastChange.

Verification on this commit: npm run test:unit 27/27 pass, npm run typecheck clean, npm run lint clean, npx prettier --check 'src/**/*.ts' clean. Changeset present; patch is the right bump.

github run

Blanket startInFlight early-return in syncPendingStoppedEvents() (src/lib/container.ts:2129) — still the highest-severity item. start() never drains pending stop events, so an alarm firing during a start skips the previous run's onStop (and its real exit code); once the start succeeds, container.running is true and no later alarm can re-detect it. It also makes stop() (line 1022) a silent no-op for pending stop events while a start is in flight. Narrowing the guard to the healthy/running branch keeps the race fix — the stopped_with_code branch is already race-safe via setStoppedIfUnchanged. Suggestion in review https://github.com/cloudflare/containers/pull/243#pullrequestreview-4995678929.

After some investigation, I think its a fair trade-off. If there is an inflight start and we haven't been able to call onStop, it feels like a fair trade-off as we would get into recursive onStop territory here (onStop calls start(), etc).